-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: async voice messages recording #2339
Conversation
# Conflicts: # src/context/ComponentContext.tsx
<button | ||
className='str-chat__attachment-preview-delete' | ||
data-testid='file-preview-item-delete-button' | ||
disabled={attachment.$internal?.uploadState === AttachmentUploadState.UPLOADING} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this $internal
stuff? Is it something voice message related? I haven't seen it for attachments before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This are internal metadata that are not shared with the back-end. I decided to keep them "isolated" under this key and just remove them before sending the message with the attachment to the back-end. Current implementation with image and file uploads seems to me clunky in that it does not isolate these locally important metadata and even use different names for those attributes, that are later removed before the submission.
I would like to propose this approach also to other types of attachments gradually to make our code more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, here's what worries me: integrators implementing custom attachment previews shouldn't care about what is and what isn't part of our backend schema. But in this case they will have to use this $internal
property, and they may find it a bit weird.
src/components/MessageInput/types.ts
Outdated
@@ -3,6 +3,12 @@ import type { DefaultStreamChatGenerics } from '../../types/types'; | |||
|
|||
type AttachmentLoadingState = 'uploading' | 'finished' | 'failed'; | |||
|
|||
export enum AttachmentUploadState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this different from UploadState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to gradually replace the union type with enum that could be used by the integrators too - if they want to do some customizations. Removing the original would be breaking. WDYT, how could this be approached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know, the union type doesn't sound bad, can we just keep using the UploadState
union in this case? Anyway, I don't have any strong opinion here.
@myandrienko thank you for the review. I forgot to put this into draft state when creating this PR. I plan to clean up the stuff still 🙂 |
Size Change: +305 kB (+17%) Total Size: 2.12 MB
ℹ️ View Unchanged
|
src/components/MediaRecorder/classes/MediaRecorderController.ts
Outdated
Show resolved
Hide resolved
src/components/MediaRecorder/classes/MediaRecorderController.ts
Outdated
Show resolved
Hide resolved
* @param file | ||
*/ | ||
export const toAudioBuffer = async (file: File) => { | ||
const audioCtx = new AudioContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we close this context eventually? Does this create a leak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by closing the context: dab90e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this: dab90e2#comments
# Conflicts: # src/components/Attachment/__tests__/Audio.test.js # src/i18n/de.json # src/i18n/en.json # src/i18n/es.json # src/i18n/fr.json # src/i18n/hi.json # src/i18n/it.json # src/i18n/ja.json # src/i18n/ko.json # src/i18n/nl.json # src/i18n/pt.json # src/i18n/ru.json # src/i18n/tr.json
docusaurus/docs/React/components/message-input-components/audio-recorder.mdx
Outdated
Show resolved
Hide resolved
docusaurus/docs/React/components/message-input-components/audio-recorder.mdx
Outdated
Show resolved
Hide resolved
🎉 This PR is included in version 11.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎯 Goal
Introduce
🛠 Implementation details
Provide a description of the implementation
🎨 UI Changes
Please see the attached documentation files.